Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement PNI #285

Merged
merged 34 commits into from
Mar 29, 2024
Merged

Implement PNI #285

merged 34 commits into from
Mar 29, 2024

Conversation

rubdos
Copy link
Member

@rubdos rubdos commented Jan 14, 2024

Fix #206, continuation of #275

TODO:

  • Update pre key bundle should take into account last-resort Kyber pre keys
  • Linking manager should use the account manager for pre key generation
  • Send the actual PNI distribution message
  • Use the supplied identity key pair in the refactored linking code
  • Consider refactoring NewDeviceRegistration to use IdentityKey instead of PublicKey or even straight IdentityKeyPair. Strong types ftw.

Maybe TODO:

  • Phone number sharing is something that's stored on the profile. Add to the profile cipher.

@rubdos
Copy link
Member Author

rubdos commented Jan 14, 2024

@gferon: in the linking fix #271, you set some Kyber last resort keys:

        let aci_pq_last_resort_pre_key =
            generate_last_resort_kyber_key(aci_store, &aci_key_pair).await?;
        let pni_pq_last_resort_pre_key =
            generate_last_resort_kyber_key(pni_store, &pni_key_pair).await?;

The problem is that the storage does not realize this is a last resort key, and we should probably make sure to tag it as such.

Second, this pre key generation code is duplicate with AccountManager, we might want to clean that up.

@gferon
Copy link
Collaborator

gferon commented Jan 14, 2024

@gferon: in the linking fix #271, you set some Kyber last resort keys:

        let aci_pq_last_resort_pre_key =
            generate_last_resort_kyber_key(aci_store, &aci_key_pair).await?;
        let pni_pq_last_resort_pre_key =
            generate_last_resort_kyber_key(pni_store, &pni_key_pair).await?;

The problem is that the storage does not realize this is a last resort key, and we should probably make sure to tag it as such.

Second, this pre key generation code is duplicate with AccountManager, we might want to clean that up.

I remember adding a comment about this but didn't see some bad side effects 😅 and decided to go forward with the fix since everything was broken. Thanks for that!

Copy link

codecov bot commented Jan 30, 2024

Codecov Report

Attention: Patch coverage is 0% with 29 lines in your changes are missing coverage. Please review.

Project coverage is 3.34%. Comparing base (a2e7540) to head (d6a6fc2).
Report is 11 commits behind head on main.

❗ Current head d6a6fc2 differs from pull request most recent head 4255520. Consider uploading reports for the commit 4255520 to get more accurate results

Files Patch % Lines
libsignal-service/src/push_service.rs 0.00% 24 Missing ⚠️
libsignal-service/src/pre_keys.rs 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #285       +/-   ##
==========================================
- Coverage   17.12%   3.34%   -13.78%     
==========================================
  Files          38      37        -1     
  Lines        3078    2898      -180     
==========================================
- Hits          527      97      -430     
- Misses       2551    2801      +250     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rubdos
Copy link
Member Author

rubdos commented Jan 30, 2024

1c3d6dd refactors this, but is currently incorrect, as it relies on the AccountManager having knowledge of the newly provided identity key pairs.

EDIT c2ceb80 should fix that.

@rubdos rubdos marked this pull request as ready for review March 9, 2024 20:38
@rubdos
Copy link
Member Author

rubdos commented Mar 9, 2024

Good for a first look. We'll test it thoroughly (as thoroughly as this complex thing can get tested...) with Whisperfish before getting this in.

local_aci: ServiceAddress,
local_pni: ServiceAddress,
aci_identity: IdentityKeyPair,
pni_identity: Option<IdentityKeyPair>,
device_id: DeviceId,
) -> Self {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since messages are always sent using the account identity[citation needed], we could instead pass the Aci and Pni stores separately into the MessageSender, and instantiate the ServiceCipher internally? That could reduce the number of arguments, because we won't have to pass the aci_identity, pni_identity nor the cipher anymore.

@rubdos
Copy link
Member Author

rubdos commented Mar 21, 2024

Imagine. This works.

@gferon
Copy link
Collaborator

gferon commented Mar 22, 2024

Amazing, I'll integrate and test it in presage today!

@rubdos
Copy link
Member Author

rubdos commented Mar 22, 2024

Amazing, I'll integrate and test it in presage today!

Quite a bit of work on the caller side, as usual, you can check out WF:
https://gitlab.com/whisperfish/whisperfish/-/merge_requests/550

@direc85
Copy link
Contributor

direc85 commented Mar 23, 2024

I've been using this with Whisperfish for a day or two now, and there haven't been anything weird to report about (after the initial shenanigans)!

@gferon
Copy link
Collaborator

gferon commented Mar 23, 2024

Just started the integration with presage, may I push small adjustment commits directly on this branch?

@direc85
Copy link
Contributor

direc85 commented Mar 23, 2024

I guess so, further fixes are always welcome. I'd still prefer the trial-and-error part happening in a fork or local check-out. Having said that, me and Ruben did exactly that a few days back 😅 Some final cleanup may take place regardless.

@rubdos
Copy link
Member Author

rubdos commented Mar 24, 2024

Just started the integration with presage, may I push small adjustment commits directly on this branch?

As long as you don't squash anything, I think that'd be fine.

@rubdos
Copy link
Member Author

rubdos commented Mar 29, 2024

@gferon I'm going to take the liberty to pull this in; @direc85 and I have been daily-driving this. It's not complete, but from here it's all incremental anyway.

@gferon
Copy link
Collaborator

gferon commented Mar 29, 2024

Go for it, I have a bunch of changes that I'll try to commit in separate changes, since they are mostly quality of life improvements and I went a little bit extra with my changes 😂

@rubdos rubdos enabled auto-merge March 29, 2024 12:00
@rubdos rubdos merged commit 93c23cf into main Mar 29, 2024
15 of 19 checks passed
@rubdos rubdos deleted the aci-pni branch March 29, 2024 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for ACI/PNI
3 participants